Constraint refactoring by ksaito7 · Pull Request #1415 · cmu-db/peloton

ksaito7 · 2018-06-18T21:13:44Z

This PR refactors constraint stuff related to planner, executor, storage and catalog.

- Contributions

Add a new catalog table for constraints: pg_constraint
Clean up data structures of constraints and its usage:
Table constraint (Primary key, Foreign key, Unique, Check) -> Constraint (pg_constraint)
Column constraint (Not null, Default) -> Column (pg_attribute)
Deleted class -> ForeignKey, MultiConstraint
Add set/add/drop constraint functions in Catalog.
Simplify checking of constraints in DataTable when inserting/updating.

- Related Issues

Store foreign keys into a catalog table #1270

- Remaining Issues

Parser does not support multiple column UNIQUE constraint.
CHECK constraints doesn't work when inserting/updating.
CHECK constraints doesn't support multiple columns or complex formula.
DDL: ADD/SET/DROP constraints are not supported.

coveralls · 2018-06-18T22:46:18Z

Coverage increased (+0.08%) to 76.512% when pulling 25a3d0c on ksaito7:pg_constraint into c949481 on cmu-db:master.

tli2 · 2018-06-19T14:42:21Z

@ksaito7 It looks like it will conflict with #1414 like crazy. Do you mind if you rebase on that PR locally?

tli2 · 2018-06-19T14:43:09Z

@apavlo Joy wrote the original code and nobody has touched that code in a long time. I don't know if anybody else understands it so can you take a look?

ksaito7 · 2018-06-19T14:49:22Z

@tli2 Sure, I will rebase locally, and fix all tabs.

db-ol

Remove redundant calls and unused variables. Revise incorrect comments. And some possible testing improvements.

db-ol · 2018-06-23T03:13:38Z

+
+  // Insert column info into each catalog
  oid_t column_id = 0;
+  std::vector<oid_t> pkey_attrs, unique_attrs;


The variables are unused.

Delete them.

db-ol · 2018-06-23T03:14:38Z

+  // Insert column info into each catalog
  oid_t column_id = 0;
+  std::vector<oid_t> pkey_attrs, unique_attrs;
+  std::shared_ptr<Constraint> pkey_constraint, unique_constraint;


The variables are also unused.

Delete them.

db-ol · 2018-06-24T17:20:49Z

+                    ->GetSchema();
+
+  // Create index
+  std::stringstream index_name(table_object->GetTableName().c_str());


Redundant call to c_str()

db-ol · 2018-06-24T17:21:04Z

+          src_table_oid, txn);
+  auto src_schema = src_table->GetSchema();
+
+  std::stringstream index_name(src_table_object->GetTableName().c_str());


Redundant call to c_str()

db-ol · 2018-06-24T23:27:35Z

+
+/*@brief    Insert a constraint into the pg_constraint table
+ *          This targets PRIMARY KEY, FOREIGN KEY, UNIQUE or CHECK constraint
+ * @param   table_oid  oid of the table related to this constraint


table_oid is not in the parameters.

Delete redundant comments.

db-ol · 2018-06-27T16:55:28Z

+  auto salary = catalog->GetTableObject("emp_db", DEFAULT_SCHEMA_NAME,
+                                        "salary_table", txn);
+
+  catalog->AddPrimaryKeyConstraint(emp->GetDatabaseOid(), emp->GetTableOid(),


Why add PrimaryKey constraints in the CreatingTable test and without verifying it?

These are not adding, but just fixing. These might be used in other test? Verifying for primary keys is done in ConstraintCatalogTest.

db-ol · 2018-06-27T16:55:42Z

+
+  catalog->AddPrimaryKeyConstraint(emp->GetDatabaseOid(), emp->GetTableOid(),
+                                   {0}, "con_primary", txn);
+  catalog->AddPrimaryKeyConstraint(department->GetDatabaseOid(),


Why add PrimaryKey constraints in the CreatingTable test and without verifying it?

db-ol · 2018-06-27T16:55:49Z

+  catalog->AddPrimaryKeyConstraint(department->GetDatabaseOid(),
+                                   department->GetTableOid(), {0},
+                                   "con_primary", txn);
+  catalog->AddPrimaryKeyConstraint(


Why add PrimaryKey constraints in the CreatingTable test and without verifying it?

db-ol · 2018-06-27T19:54:08Z

+  LOG_DEBUG("%s", con_table->GetSchema()->GetInfo().c_str());
+  LOG_DEBUG("Complete check for constraint table");
+
+  // Drop constraint


Would it be better if we can verify it after dropping constraints?

Correct. Add verifying.

db-ol · 2018-06-28T10:47:07Z

  table->AddIndex(pkey_index);

+  // Create primary key constraint on the table
+  if (need_primary_index) {


Should it be need_primary_index or need_primary_key?

Fix it to need_primary_key.

…to pg_constraint

This reverts commit 898219f.

* Revert "Constraint refactoring (#1415)" This reverts commit 898219f

* Add pg_constraint catalog table * Reconstruct constraints

* Revert "Constraint refactoring (cmu-db#1415)" This reverts commit 898219f

ksaito7 added 5 commits June 5, 2018 14:25

Add pg_constraint catalog table

c45dc48

Merge branch 'catalog_update'

49dfd22

Reconstruct constraints

f05450c

Merge branch 'master' into pg_constraint

f6d0336

Formatting and add constraint tests

0585694

Fix bugs in LOG_TRACE

85b6c91

tli2 requested a review from apavlo June 19, 2018 14:40

tli2 added the in progress label Jun 19, 2018

tli2 and others added 4 commits June 19, 2018 11:17

Merge branch 'master' into pg_constraint

0e2eaf9

Merge branch 'master' into pg_constraint

0934e37

Replace tab with spaces

f589361

Merge branch 'master' into pg_constraint

e5bc69d

tli2 added ready_for_review and removed in progress labels Jun 19, 2018

Fix a problem that the order of query results changes

e752d4b

apavlo requested review from db-ol and removed request for apavlo June 21, 2018 15:21

tli2 and others added 5 commits June 22, 2018 10:26

Merge branch 'master' into pg_constraint

86cd980

Merge branch 'master' into pg_constraint

47a60dc

Merge branch 'master' into pg_constraint

6abc244

Rename ConstraintCatalogObject to ConstraintCatalogEntry

f789319

Merge branch 'master' into pg_constraint

3a0d85a

db-ol suggested changes Jun 28, 2018

View reviewed changes

Merge master and cleanup for issue cmu-db#1398

0657985

tli2 added in progress and removed ready_for_review labels Jun 28, 2018

tli2 and others added 5 commits June 28, 2018 11:02

Merge branch 'master' into pg_constraint

5bd0593

Fix constraints stuff

bb11eb9

Merge branch 'pg_constraint' of https://github.com/ksaito7/peloton in…

2b3638d

…to pg_constraint

Fix DEBUG_INFO argument

c6ecd94

Fix LOG_TRACE argument

ad827a2

ksaito7 added ready_for_review and removed in progress labels Jun 29, 2018

db-ol approved these changes Jun 30, 2018

View reviewed changes

Merge branch 'master' into pg_constraint

fe5697e

tli2 added accepted and removed ready_for_review labels Jul 1, 2018

Merge branch 'master' into pg_constraint

25a3d0c

tli2 merged commit 898219f into cmu-db:master Jul 3, 2018

ksaito7 mentioned this pull request Jul 5, 2018

Settings refactoring #1432

Open

mbutrovich mentioned this pull request Jul 5, 2018

TPC-C segfault: Double-free in GCManager #1452

Closed

tli2 added a commit that referenced this pull request Jul 6, 2018

Revert "Constraint refactoring (#1415)"

864bd93

This reverts commit 898219f.

mbutrovich mentioned this pull request Jul 6, 2018

Revert 1415 #1453

Merged

tli2 pushed a commit that referenced this pull request Jul 6, 2018

Revert 1415 (#1453)

e5c2213

* Revert "Constraint refactoring (#1415)" This reverts commit 898219f

mtunique pushed a commit to mtunique/peloton that referenced this pull request Apr 16, 2019

Constraint refactoring (cmu-db#1415)

59a6e55

* Add pg_constraint catalog table * Reconstruct constraints

mtunique pushed a commit to mtunique/peloton that referenced this pull request Apr 16, 2019

Revert 1415 (cmu-db#1453)

0231468

* Revert "Constraint refactoring (cmu-db#1415)" This reverts commit 898219f

Conversation

ksaito7 commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tli2 commented Jun 19, 2018

Uh oh!

tli2 commented Jun 19, 2018

Uh oh!

ksaito7 commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

db-ol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksaito7 Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksaito7 Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ksaito7 commented Jun 18, 2018 •

edited

Loading

coveralls commented Jun 18, 2018 •

edited

Loading

ksaito7 commented Jun 19, 2018 •

edited

Loading

ksaito7 Jun 28, 2018 •

edited

Loading

ksaito7 Jun 28, 2018 •

edited

Loading